feat(plugins): add Cordis-scoped dynamic tool contributions - #4443
Conversation
|
I think the existing Maka already has a mechanism for changing the provider-visible Tool surface on the next logical model step:
This was an explicit part of the agreement in #3752:
#4098 then simplified this further: the final executable binding is the only availability authority, every non-direct bound Tool is deferred by default, and groups are only search metadata. Against that existing contract, this PR currently does something broader: That duplicates part of the existing availability mechanism and, more importantly, changes the capability model from “the Run binding is the ceiling” to “the ceiling may expand or contract during the Run.” I think that is a separate architectural decision from allowing plugins to contribute Tools. There is also a production-semantics gap in the current tests. The main dynamic plugin tests construct the backend without Suggested minimal integrationI would keep the valuable Plugin Platform work in this PR:
But I would connect it to Runtime through an immutable binding snapshot rather than a live interface PluginToolSnapshot {
revision: string
tools: readonly MakaTool[]
groups: readonly ToolGroup[]
release(): void
}The Interactive Run Composer would take one plugin snapshot while constructing the backend/Run, merge its Tools and group metadata into the final executable binding, and let the existing Plugin package/entry identity can naturally provide search-source metadata, for example: On install/enable/uninstall, the Plugin Platform updates canonical state and invalidates idle backends. The active Turn keeps its pinned snapshot; the next Turn receives the new binding. Uninstall can remove the entry from future snapshots immediately while reporting cleanup pending until snapshot references and active calls drain. This fits the existing This path would avoid:
If same-Turn install-and-use is a hard requirementThat is a valid but stronger feature. It should be stated as an explicit change to the #3752 capability contract. The ceiling would no longer be a fixed executable Tool set; it would become a fixed set of trusted Tool sources whose contents may change. Even in that design, I do not think the best seam is “re-resolve every Tool before every step.” A more coherent extension would make the Plugin Tool registry a dynamic source behind
This preserves the existing lazy-loading model and makes the additional authority explicit instead of introducing a parallel dynamic-composition path. My recommendation is therefore to start with the snapshot/binding integration and next-Turn mutation semantics. If the required product behavior is specifically “the model installs or authors a plugin and invokes it in the same Turn,” that should be separated and reviewed as a dynamic Tool-source/capability-ceiling change. Without that requirement, most of the per-step composition and persistence work in this PR appears unnecessary. The key decision to settle before continuing is: must plugin installation or removal affect the Turn that is currently running? |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 271fa3e3. Thanks for this — the scoped registry is the right shape, and I like that Profile/Session layering resolves through one registry instead of two lookups. The Fiber-owned lifetime and the Plugin Platform transaction hook are also the parts I'd have worried about most, and they hold up: I traced a dispose during an in-flight call and the retired entry throws rather than silently swapping an implementation, so a Turn can't end up executing a Tool it never advertised.
I also want to say up front that I'm not questioning the per-step epoch design. recordRequestComposition's own comment says it's matching DSH request/header epochs, and re-sampling the surface between logical steps is exactly what makes live registration meaningful. Everything below takes that as given.
Four things, one of which I think should block merge — but it's the one that's easiest to fix, because it isn't part of this feature at all.
P0 — the RunComposition v1→v2 change will stop the Host from starting on any existing install. This is separable from the plugin work, and I think dropping it from this PR is cheaper than adding a migration. Details inline on run-composition.ts.
P2 — Host-owned core Tools cannot be shadowed isn't true on the toolProfile path. The summary states this as a property of the change, and it holds for boundTools, but the two guards in interactive-run-composer.ts are checking different things on adjacent lines. Inline.
P2 — the best-effort store latch now sits in front of provider dispatch. You followed the file's existing pattern here and I don't think that was the wrong instinct; the difference is what's downstream of it. Inline.
P2 — epoch equality only compares against the previous epoch, so an oscillating surface re-appends in full. Inline, with the numbers.
A couple of smaller notes I don't think are worth inline threads:
toolNamesis capped at 256 andtoolSchemasat 512, but both are derived from the sameactiveToolsForRequest, so the real cap is 256 and nothing upstream enforces it. Since the summary pitches adapting large catalogues (the 59-Tool dsh-quant example), it's worth aligning those two and deciding whether hitting the cap should truncate the record or fail the step. Same question for MCP tool descriptions —boundedString(schema.description, 16_384)in the snapshot, no length bound at all inmcp-tools.tswhere the description comes straight from the server. I have no evidence a real server exceeds 16 KB, so this is a "which side should give" question rather than a reported bug.toolAvailabilityHashin the per-step epoch readsthis.input.toolAvailability, frozen at backend construction, while the catalogue is now re-sampled each step throughresolveTools(). The real change is already covered bytoolCatalogHash/toolNames/toolSchemas, so nothing is wrong — the field just can't do what its name promises in a per-step record.
One thing worth knowing about plugin-tool-service.test.ts: the conflict case ('desktop-ui and Host-owned Tool conflicts fail closed') calls tools.resolve('alpha', [tool('Read', 'host')]) with an explicit core list, but production calls pluginTools.resolve(sessionId, []) at execution-composition.ts:700. So the guard that test exercises never runs in production, which is why the shadowing path below is green. None of the three test files go through createInteractiveRunComposer; one test that does would cover the second finding directly.
Evidence boundary: I read pr4443 against origin/main and ran the PR's own run-composition.ts + record-schema.ts in isolation to check the decode both directions. The startup consequence in the P0 is traced through the call chain and through Desktop's startDesktopRuntimeHostWithRecovery, not reproduced end to end — I did not stand up an old database and watch a Host fail to start. The 44 KB/epoch figure is measured from 40 real Tools in a main build, so it excludes MCP and plugin contributions and is a lower bound. I did not run the test suites.
AI-assisted review: drafted with Maka; I verified the decode failure, the two guards, the latch's callers, and the shadowing path against the branch source myself.
| import { defineObjectShape, hasExactShape, isRecord } from './record-schema.js'; | ||
|
|
||
| export const RUN_COMPOSITION_SCHEMA_VERSION = 1 as const; | ||
| export const RUN_COMPOSITION_SCHEMA_VERSION = 2 as const; |
There was a problem hiding this comment.
P0 — every existing database has v1 rows, and v2 rejects them on a read path that runs during Host startup.
Two independent reasons a v1 row fails now: the version literal moved to 2, and RUN_COMPOSITION_SHAPE dropped sourceRevisions, baseSystemPromptHash, toolCatalogHash, toolAvailabilityHash and toolNames with an empty optional list, which hasExactShape treats as excess keys. I pulled this file and record-schema.ts out and ran them:
v1 record REJECTED Invalid Run Composition snapshot schema
v1 shape @ version 2 REJECTED Invalid Run Composition snapshot schema
v2 record decoded OK
main decoding v2 REJECTED Invalid Run Composition snapshot schema
So it breaks in both directions, and bumping only the version isn't enough to make an old row readable.
Every real Run has one of these: commitRunComposition is on beforeRunProviderDispatch unconditionally (execution-model-composition.ts:331), and the commit that introduced v1 is an ancestor of v0.2.0-incubating-rc1.
The consequence isn't a silent downgrade. decodeRunCompositionSnapshot throws, isRunCompositionSnapshot returns false, and agent-run.ts:707 throws Invalid AgentRun header schema. That surfaces during recovery with no per-row catch anywhere on the way:
agent-run-store.ts:538 rows.map(row => decodePersistedAgentRunHeader(...)) // bare map
-> listSessionRunsForRecovery -> hosted-execution-recovery -> prepareRecovery
-> execution-composition.ts:1776 -> host-kernel.ts:387 await this.#composition.recover()
host-kernel.ts:387 has a finally and no catch, so the rejection crosses Promise.race and #state never reaches 'ready'. On the Desktop side, startDesktopRuntimeHostWithRecovery rethrows anything canRepairManagedRuntimeHostStartup doesn't recognise, and that predicate only accepts RuntimeHostStartupError with one of seven deployment reasons — a raw schema error isn't among them, so the repair prompt isn't even offered. One old row, no start, no in-product way out. listSessionRunsPage, listSessionRunsBounded and readRun take the same path, and readSqliteAgentRunEvents:979 reads the header first, so events go with it.
The cheapest fix is probably to take this out of this PR. Dynamic plugin Tools don't need those five fields removed from RunCompositionSnapshot — as far as I can tell nothing else in the diff depends on the narrower shape. Landing the feature without the schema change means no migration to write and no P0.
If you'd rather keep it, the repo already has the seam: decodePersistedAgentRunHeader (agent-run.ts:620) is defined by its own comment and tests as the persistence boundary where retired values get folded — automationId and waiting_permission both go through it — while decodeAgentRunHeader stays strict about the current shape. A v1→v2 fold there (drop the removed fields, set schemaVersion: 2) is a few lines, and AGENT_RUN_CONTINUATION_SOURCE_V1_SHAPE/V2_SHAPE in the same file is the existing precedent for discriminated decoding.
Worth flagging either way: the only test that guarded this now asserts the new behaviour. sqlite-core-execution-store.test.ts:733's fixture was updated from schemaVersion: 1 to 2, so no test in the tree constructs a v1 record any more, and run-composition.test.ts asserts that v1-shaped input must throw.
There was a problem hiding this comment.
Fixed in 714bfc2. RunComposition is restored to the exact persisted v1 shape and semantics; the SQLite fixture is back on v1 and the decoder regression now explicitly accepts v1 and rejects v2. Dynamic request surfaces remain separate RequestComposition records, so existing databases require no migration.
| const tools = [...selectedTools]; | ||
| assertUniqueToolNames(tools); | ||
| const resolveTools = (): readonly MakaTool[] => { | ||
| const additionalTools = input.boundTools ? [] : (input.resolveAdditionalTools?.() ?? []); |
There was a problem hiding this comment.
P2 — this guard and the one at :153 are answering different questions, which is how plugin Tools reach toolProfile sessions.
136 const hasToolCeiling = input.boundTools !== undefined || input.toolProfile !== undefined;
139 const additionalTools = input.boundTools ? [] : (input.resolveAdditionalTools?.() ?? []);
153 const clientCapabilityTools = hasToolCeiling ? [] : (input.clientCapabilities?.tools ?? []);Client capability Tools are excluded from toolProfile sessions; plugin Tools aren't, because :139 only looks at boundTools. I think :139 wants hasToolCeiling too, and that's the whole fix.
What follows from it is why I'm raising it rather than leaving it as a nit. The summary says Host-owned core Tools cannot be shadowed, and PluginToolService does guard that — but only when it's handed the core list, and production calls pluginTools.resolve(sessionId, []) at execution-composition.ts:700, so that guard never fires. Downstream:
additionalToolslands inbuildDefaultHostTools(..., [...hostTools, ...additionalTools], ...)at:146, after the builtins.projectHostedExecutionToolsbuildsnew Map(tools.map(tool => [tool.name, tool]))— last wins — so a pluginBashdisplaces the Host one.selected = toolNames.map(name => byName.get(name))picks the plugin entry.- Then:
tool.name === 'Bash'
? { ...tool, description: HEADLESS_CODING_V1_BASH_DESCRIPTION,
parameters: HEADLESS_CODING_V1_BASH_PARAMETERS }
: tool...tool keeps the plugin's impl while description and parameters are overwritten with the Host contract. The model is shown the Host's Bash schema and calls the plugin's implementation against it.
assertUniqueToolNames(resolved)runs after the Map has already collapsed the duplicate, so it can't see the collision.
I graded this P2 rather than higher because Host plugins are trusted and already run arbitrary code in the Host process, so shadowing Read grants no capability they didn't have; uninstalling restores the original, and nothing persisted or externally visible changes. What makes it worth fixing before merge is that it's silent — a plugin author who picks a colliding name gets a schema/implementation mismatch with no diagnostic, and the comment above assertUniqueToolNames describes exactly the invariant that's being missed here.
There was a problem hiding this comment.
Fixed in 714bfc2. resolveAdditionalTools now uses the same hasToolCeiling guard as Client Capability Tools, covering both boundTools and toolProfile. Added an interactive composer regression proving an explicit profile excludes plugin additions and preserves exactly one Host Read binding.
| if (!this.input.runStore) { | ||
| throw new Error('AgentRun store is not configured'); | ||
| } | ||
| if (!this.runStoreAvailable) throw new Error('AgentRun store is unavailable'); |
There was a problem hiding this comment.
P2 — this is the same latch check as :554 and :578, but it's the first one in front of provider dispatch.
You followed the file's existing pattern and I don't think that was wrong — recordModelProjectionTransition and recordHistoryCompactCheckpoint both open with this line on main. The difference is what happens when it trips. Those two are recorders whose failure the caller can absorb; recordRequestComposition is awaited before every provider request, so a false latch fails the step itself.
runStoreAvailable is best-effort state: enqueueRunStore:1643 clears it on any trace-append failure, and a busy SQLite is enough. Because the throw happens before enqueueRequiredRunStoreWrite runs, the probe that would lift the latch back never executes, so a single transient hiccup turns into "every remaining step of this Run fails before dispatch" with no self-repair.
recordRunComposition right above shows the shape that avoids this: it goes straight to enqueueRequiredRunStoreWrite, whose comment spells out the reasoning — a successful required write proves the store is available again. Dropping :476 and letting the required-write path do its own probing gets the same durability with a recoverable failure mode.
There was a problem hiding this comment.
Fixed in 714bfc2. The best-effort runStoreAvailable precheck is removed; Request Composition goes through enqueueRequiredRunStoreWrite directly, so a successful required write lifts the latch while dispatch remains fail-closed on a real write failure.
| input, | ||
| this.requestComposition ? 'change' : 'initial', | ||
| ); | ||
| if ( |
There was a problem hiding this comment.
P2 — comparing only against the previous epoch means an oscillating surface appends a full replacement every step.
sameRequestCompositionSurface(this.requestComposition, snapshot) looks at the most recent epoch only, so an A→B→A→B sequence writes four full records. activeToolsForRequest does move around within a Turn — repair plans, sandbox boundary finalization and the final child summary step all rewrite it (ai-sdk-backend.ts:2101-2105).
Serializing 40 real Tools from a main build the way the snapshot does gives 44,049 characters per epoch (maka_computer alone is 14 KB, Bash 3.1 KB), before any MCP or plugin Tools. Against EXECUTION_INSPECT_EVIDENCE_MAX_BYTES = 512 * 1024 — shared between AgentRun and runtime events, accumulated by stored_bytes in bounded-evidence.ts:58 — that's roughly 11 epochs before the Run returns limit_exceeded and InspectQueryTooLargeError tells the operator to stop the Host and inspect offline. These events are append-only with no compaction and no cleanup path, and every whole-ledger loader (history-compact-ledger.ts:62, canonical-turn-snapshot.ts:56, conversation-copy.ts:281, …) parses and discards them.
Also worth noting that this.requestComposition is memory-only and never rehydrated from the ledger, so each resume writes a fresh initial epoch even when the surface is identical to what the previous instance recorded. That makes reason less reliable as a ledger fact than it reads — the model-call-attempt.ts:158-159 comment currently suggests every step carries an id, but compaction and memory sub-calls build their own tracker at ai-sdk-backend.ts:3303 and leave requestCompositionId undefined even on a fully upgraded Run.
Deduplicating against every epoch already in this Run — or storing toolSchemas once per (runId, surfaceHash) and having epochs reference it — would keep the DSH-style per-step record without the growth. Since nothing in the tree reads request_composition_resolved or dereferences requestCompositionId outside tests yet, there's also room to store just the hash for now and add the full schemas when a reader needs them.
There was a problem hiding this comment.
Fixed in 714bfc2. Request Composition snapshots are now indexed by a canonical full-surface hash across the entire Run, with existing epochs rehydrated from the ledger. A→B→A and reopening the same Run both reuse the original composition id; ModelCallAttempt references retain the step timeline without repeating full schemas.
|
Same-Turn install/enable/use is a hard requirement for this PR, so the per-step dynamic source remains. The compatibility follow-up in 714bfc2 keeps RunComposition v1 unchanged, moves all dynamic evidence to immutable logical-step RequestComposition snapshots, preserves retry freezing and fail-closed dispatch, prevents same-name replacement from inheriting an old tool_search activation, and keeps explicit bound/profile ceilings exact. Full workspace lint/typecheck and 348 focused regressions pass. |
|
Followed up on the two non-blocking composition-bound notes in fadbd38: RequestComposition now applies the same fail-closed 256-entry bound to both toolNames and toolSchemas (and the same 128-character name bound), so exact evidence is never truncated. MCP descriptions are normalized to the persisted 16 KiB bound before becoming provider-visible, while trusted Plugin Tool registration rejects empty or oversized descriptions atomically. Added focused boundary regressions; full lint, all-workspace typecheck, and 253 related tests pass. |
|
There is a blocking regression in the latest head: the per-step resampling now breaks existing The new replacement guard in for (const [name, activatedTool] of activeTools) {
if (this.toolsByName.get(name) !== activatedTool) activeTools.delete(name)
}That is not compatible with the current composer lifecycle.
The resulting sequence is deterministic: So The plugin weather scenario does not disprove this. Object reference is therefore not a valid general contribution identity. The minimal coherent fix is to keep the base Host binding stable and resample only dynamic plugin contributions, while carrying an explicit activation identity: A same-name plugin replacement should invalidate activation because its contribution identity changed. An unchanged static Tool must retain activation even if an implementation currently rebuilds its wrapper. Please add a production-path regression through
The compatibility fixes in |
|
@likun666661 Fixed the blocking The per-step dynamic Tool projection remains. The base Host binding is now stable for the backend, scoped Plugin contributions are still re-sampled at each logical step, and Turn activation stores a stable logical key instead of a JavaScript object reference. Host wrappers fall back to their canonical provider-visible shape; Plugin Tools bind Added production-path coverage for:
Validation on current |
|
Follow-up Tool-only hardening in |
|
I did an Occam pass on the current head ( With that requirement, I think the minimal problem is now well-defined:
The causal path is correspondingly small: The current head now implements that path coherently:
One production-semantics detail is worth making explicit in the framing. With ordinary deferred Tool availability enabled, same-Turn installation does not mean that the complete schema is automatically visible on the immediately following request. The normal sequence is: That still satisfies same-Turn install/use and preserves the existing lazy-loading contract. From an Occam perspective, my conclusion is:
So my recommendation is to keep the current behavior, tighten the Summary around the minimal problem above, and treat any static-index/dynamic-source split as a measured follow-up rather than expanding this PR further. |
…hots Consolidate the reviewed PR apache#4443 implementation and prior integration fixes before rebasing onto current main. Generated-by: Codex
480b147 to
7dd815f
Compare
3 participants
Summary
This PR supports dynamic scoped Tool contributions from trusted Host plugins within an active Turn. A plugin may add, replace, or remove Tools; the change takes effect at the next logical model-step boundary. The current provider request, all physical retries, and returned Tool calls remain bound to the immutable step-start Tool snapshot. Explicit
boundToolsand Tool profiles remain exact capability ceilings.The runtime path is:
ctx.tools.register()for trusted Host plugins, with registration/disposal owned by the registering Fiber and the Plugin Platform transaction.tool_searchactivation.tools/changenotifications transactionally and roll publication back if a listener rejects the change.Runtime semantics
Same-Turn install/use preserves the existing deferred Tool availability contract. Installation does not automatically expose the complete schema on the immediately following request. With ordinary deferred availability enabled, the normal sequence is:
At each logical-step boundary, Runtime re-samples scoped contributions and freezes the effective request surface. Returned calls resolve against the
providerToolssnapshot advertised for that step; physical retries reuse that same snapshot and request-composition identity.Disposal removes the contribution from subsequent step surfaces. Already-active calls drain, while stale starts are rejected. Exact bound Tool lists and explicit Tool profiles continue to limit which contributions may be exposed or invoked.
Composition and persistence
RunCompositionremains the immutable initial C0 baseline, written before the first provider dispatch. Its persisted v1 schema and semantics are unchanged, and existing databases decode without migration.Per-step request surfaces are captured separately in
request_composition_resolved, including Prompt source revisions and hash, Tool catalog and availability hashes, active Tool names, canonical provider-visible schemas, and the provider-options hash. Prompt capture is supporting request/audit context; the feature's core requirement is dynamic scoped Tool contributions with immutable logical-step surfaces.The current request and all physical retries bind one immutable
requestCompositionId. Snapshots are deduplicated by their complete canonical surface across the Run, including after reopening: A → B → A reuses A.ModelCallAttemptreferences retain the per-step timeline.Both composition writes remain fail-closed before provider dispatch. Required Request Composition writes bypass the best-effort store latch so a transient trace-write failure can recover.
Scope and follow-up
This provides runtime and Plugin Platform primitives for installable Tool packages, Profile/Session-scoped contributions, and same-Turn enable/discover/use flows. Only trusted Host plugins can contribute Tools.
Model-authored self-extension and external compatibility probes are capability demonstrations and audit context, not requirements of this feature. A built-in Tool generator, model-authored code execution, sandbox policy, artifact persistence, quotas, and approval UX are outside this PR's scope.
The implementation rebuilds the complete
ToolAvailabilityRuntime/MiniSearch projection at each logical step. A stable Host index plus a dynamic Plugin source behindtool_searchis a possible follow-up if profiling demonstrates meaningful cost; this PR does not expand into that optimization.Verification
Automated regression
npm run typechecknpm run lintReal Maka + real model
The live scenarios were exercised with DeepSeek V4 Flash on the same dynamic Tool implementation before the compatibility follow-up:
committed + convergedcommitted + converged + cleanup complete; the catalog returned to zero plugin ToolsRun Composition changed after resolutionfailure no longer occursOut-of-tree compatibility and capability probes
These probes are not included in this PR; they use only the public behavior introduced here:
dsh-quantTools without modifying Maka, installed them through the real Plugin Platform, invoked a deterministic calculation, then uninstalled them with 0 Tools remainingThese previously reported checks are supporting verification and capability context; the out-of-tree probes do not broaden the core requirement above.